Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start development of a new pretty printer for Cadence programs #1024

Merged
merged 35 commits into from
Nov 30, 2021

Conversation

turbolent
Copy link
Member

Work towards #209

Description

Start the development of a new pretty-printer for Cadence.

Implement pretty-printing functions for the following AST nodes:

  • All expressions:
    • ArrayExpression
    • BoolExpression
    • DestroyExpression
    • DictionaryExpression
    • ForceExpression
    • IdentifierExpression
    • IndexExpression
    • MemberExpression
    • NilExpression
    • PathExpression
    • StringExpression
    • UnaryExpression
    • InvocationExpression
    • IntegerExpression
    • CreateExpression
    • FixedPointExpression
    • ConditionalExpression
    • BinaryExpression
    • CastingExpression
    • ReferenceExpression
    • FunctionExpression
  • Declarations:
    • VariableDeclaration
  • Statements:
    • ReturnStatement

Still missing for expressions:

  • Types
  • Precedence: Parentheses
  • Function pre and post-conditions
  • Optional: Replace the String() functions with pretty-print results (assume a default max line width)

Also, implement a small web-based tool that helps with the development: It parses code into the AST and pretty-prints it, given a maximum line width.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@turbolent turbolent self-assigned this Jun 20, 2021
@turbolent turbolent changed the title Bastian/prettier Start development of a new pretty printer for Cadence programs Jun 20, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2021

Codecov Report

Merging #1024 (13f0b32) into master (0e7825d) will decrease coverage by 0.08%.
The diff coverage is 79.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1024      +/-   ##
==========================================
- Coverage   77.31%   77.22%   -0.09%     
==========================================
  Files         274      278       +4     
  Lines       35347    35814     +467     
==========================================
+ Hits        27327    27659     +332     
- Misses       6941     7069     +128     
- Partials     1079     1086       +7     
Flag Coverage Δ
unittests 77.22% <79.69%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/ast/transfer.go 65.00% <0.00%> (-35.00%) ⬇️
runtime/ast/variable_declaration.go 40.42% <0.00%> (-35.58%) ⬇️
tools/pretty/main.go 0.00% <0.00%> (ø)
runtime/ast/string.go 89.28% <89.28%> (ø)
runtime/ast/expression.go 82.87% <96.88%> (+8.75%) ⬆️
runtime/ast/block.go 90.00% <100.00%> (+2.50%) ⬆️
runtime/ast/statement.go 78.26% <100.00%> (+0.69%) ⬆️
runtime/ast/type.go 83.76% <100.00%> (+0.21%) ⬆️
runtime/format/string.go 100.00% <100.00%> (+10.71%) ⬆️
runtime/parser2/expression.go 93.67% <100.00%> (+0.01%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e7825d...13f0b32. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Nov 25, 2021

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 0e7825d
The command go test ./... -run=XXX -bench=. -shuffle=on -count N was used.
Bench tests were run a total of 7 times on each branch.

Results

old.txtnew.txt
time/opdelta
RuntimeStorageWriteCached-2151µs ± 5%173µs ± 8%+14.77%(p=0.001 n=7+7)
QualifiedIdentifierCreation/One_level-23.39ns ± 2%3.50ns ± 1%+3.24%(p=0.003 n=7+5)
RuntimeFungibleTokenTransfer-21.51ms ±27%1.64ms ±23%~(p=0.535 n=7+7)
RuntimeResourceDictionaryValues-218.0ms ± 6%18.5ms ± 6%~(p=0.165 n=7+7)
ParseDeploy/byte_array-237.0ms ± 5%38.0ms ± 4%~(p=0.128 n=7+7)
ParseDeploy/decode_hex-21.59ms ± 7%1.56ms ± 3%~(p=0.295 n=7+6)
ParseFungibleToken-2507µs ± 6%527µs ± 5%~(p=0.209 n=7+7)
ParseInfix-226.6µs ± 6%26.9µs ± 4%~(p=0.456 n=7+7)
ParseArray-224.9ms ± 3%25.3ms ± 3%~(p=0.259 n=7+7)
QualifiedIdentifierCreation/Three_levels-2173ns ± 3%176ns ± 2%~(p=0.175 n=7+7)
CheckContractInterfaceFungibleTokenConformance-2186µs ± 5%191µs ± 4%~(p=0.101 n=7+6)
ContractInterfaceFungibleToken-251.8µs ± 6%52.1µs ± 2%~(p=0.628 n=7+6)
NewInterpreter/new_interpreter-21.34µs ± 4%1.36µs ± 4%~(p=0.402 n=7+7)
NewInterpreter/new_sub-interpreter-22.45µs ± 5%2.38µs ± 3%~(p=0.053 n=7+7)
InterpretRecursionFib-23.28ms ±12%2.86ms ± 2%−12.65%(p=0.001 n=7+7)
 
alloc/opdelta
NewInterpreter/new_interpreter-2680B ± 0%720B ± 0%+5.88%(p=0.001 n=7+7)
NewInterpreter/new_sub-interpreter-21.06kB ± 0%1.11kB ± 0%+5.30%(p=0.001 n=7+7)
InterpretRecursionFib-21.21MB ± 0%1.24MB ± 0%+2.01%(p=0.001 n=6+7)
RuntimeFungibleTokenTransfer-2231kB ± 0%233kB ± 0%+0.61%(p=0.001 n=7+7)
RuntimeResourceDictionaryValues-24.34MB ± 0%4.34MB ± 0%+0.02%(p=0.001 n=7+7)
RuntimeStorageWriteCached-283.7kB ± 0%83.7kB ± 0%+0.00%(p=0.033 n=7+6)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-265.7kB ± 0%65.7kB ± 0%~(all equal)
ContractInterfaceFungibleToken-226.5kB ± 0%26.5kB ± 0%~(all equal)
 
allocs/opdelta
NewInterpreter/new_sub-interpreter-231.0 ± 0%32.0 ± 0%+3.23%(p=0.001 n=7+7)
RuntimeFungibleTokenTransfer-24.40k ± 0%4.40k ± 0%+0.07%(p=0.001 n=7+7)
RuntimeResourceDictionaryValues-2108k ± 0%108k ± 0%+0.00%(p=0.009 n=7+7)
RuntimeStorageWriteCached-21.42k ± 0%1.42k ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-21.07k ± 0%1.07k ± 0%~(all equal)
ContractInterfaceFungibleToken-2457 ± 0%457 ± 0%~(all equal)
InterpretRecursionFib-225.0k ± 0%25.0k ± 0%~(all equal)
NewInterpreter/new_interpreter-211.0 ± 0%11.0 ± 0%~(all equal)
 

Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! 🤩

One general question I had was, would it make sense to move the formatting/prettying codes to a separate visitor?

"strings"

"github.com/turbolent/prettier"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍 🎉

@@ -34,6 +35,7 @@ type Expression interface {
IfStatementTest
isExpression()
AcceptExp(ExpressionVisitor) Repr
Doc() prettier.Doc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this returns the 'textual' representation of the expression. Would it make sense to rename it to something that reflects it? People might confuse it with 'documentation/doc-comments' (happened to me: I was wondering for a moment, can there be any doc-comments for expressions 😄 )

Copy link
Member Author

@turbolent turbolent Nov 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't occur to me, bad that's a good point! Would PrettyPrintDocument() be a better name? Any other suggestions? Naming is hard, haha

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea PrettyPrintDocument() sounds a better option. Some other names I could think of are PrettyPrint(), Format().

Alternatively, if we can move this to a separate visitor, then we wouldn't have this method anyway.

One general question I had was, would it make sense to move the formatting/prettying codes to a separate visitor?

That might be a big refactor though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I had missed that comment! Good idea, I might refactor the existing code to a separate visitor once the existing PRs are in

separatorDoc = memberExpressionSeparatorDoc
}
return prettier.Concat{
// TODO: potentially parenthesize
Copy link
Member

@SupunS SupunS Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good solution is to introduce a ParenthesizedExpression to the AST at the parser level, so we don't have to specially handle parentheses everywhere. Expression.Doc() would delegate to ParenthesizedExpression.Doc() which would then add the parentheses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of-course we don't have to do it here; just a suggestion for future improvements.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting idea! 👌 I'd love to understand it better, let's have a sync next week

prettier.Group{
Doc: leftDoc,
},
prettier.Line{},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this going to be a newline always?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier.Line is rendered as ae space if there is space, or a newline if there is not.

The idea here is to break before the symbol, so that e.g. let a = 1 + 2 * 3 is rendered like this if it does not fit:

let a = 1
    + 2
    * 3

@bluesign
Copy link
Contributor

@turbolent This is amazing. Is it any how feasible to port parser to javascript? Can be useful for fcl and js-testing probably.

It is for a long time in my ideas list. If there is no obvious road blocks, I would like to work on this.

@SupunS
Copy link
Member

SupunS commented Nov 29, 2021

@bluesign Parser is already available as an npm-package: https://github.com/onflow/cadence/tree/master/npm-packages/cadence-parser

@turbolent
Copy link
Member Author

@bluesign Yeah, like @SupunS pointed out, it should be very little work to compile to expose this to e.g. JavaScript via WebAssembly, by adding functions to the parser NPM package.

I don't know what the current status is for TinyGo, but it might be able to compile the parser to a smaller binary than the reference Go compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants